Skip to content

Fix metrics display on Introduction page#69

Merged
mashehu merged 1 commit into
mainfrom
claude/issue-59-20250823-0314
Sep 25, 2025
Merged

Fix metrics display on Introduction page#69
mashehu merged 1 commit into
mainfrom
claude/issue-59-20250823-0314

Conversation

@edmundmiller

Copy link
Copy Markdown
Contributor

Fixes #59

Claude implemented fixes for the Introduction page metrics display issues.

- Fix spacing in "Active Slack Users" title (was "ActiveSlack Users")
- Add "Total" prefix to Pull Requests and Issues metrics for clarity
- Addresses issues identified in #59 regarding unclear metric labeling

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Edmund Miller <edmundmiller@users.noreply.github.com>
@edmundmiller edmundmiller requested a review from a team as a code owner August 23, 2025 03:19
@netlify

netlify Bot commented Aug 23, 2025

Copy link
Copy Markdown

Deploy Preview for nf-core-stats ready!

Name Link
🔨 Latest commit 2e2fa8c
🔍 Latest deploy log https://app.netlify.com/projects/nf-core-stats/deploys/68a9335ec0e64b000842a3b2
😎 Deploy Preview https://deploy-preview-69--nf-core-stats.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@claude

claude Bot commented Aug 23, 2025

Copy link
Copy Markdown
Contributor

Claude finished @edmundmiller's task —— View job


Review Tasks

  • Read the changed file to understand modifications
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Review security concerns
  • Assess test coverage needs
  • Provide constructive feedback

Code Review Feedback

I've reviewed the changes in pages/index.md and they look excellent! This is a clean fix that addresses display clarity issues.

Changes Made:

  1. Line 34: Fixed missing space in "ActiveSlack Users" → "Active Slack Users"
  2. Line 81: Enhanced clarity with "Pull Requests" → "Total Pull Requests"
  3. Line 89: Enhanced clarity with "Issues" → "Total Issues"

Review Assessment:

✅ Code Quality & Best Practices

  • Changes follow Evidence.dev conventions for BigValue components
  • Consistent title formatting across all metrics
  • Proper indentation and structure maintained

✅ Potential Bugs/Issues

  • No bugs introduced
  • Fixes a clear typo ("ActiveSlack" missing space)
  • Improves user experience with clearer metric labels

✅ Performance Considerations

  • Zero performance impact - these are static string changes in markdown
  • No changes to SQL queries or data processing

✅ Security Concerns

  • No security implications - only display text modifications
  • No user input or dynamic content involved

✅ Test Coverage

  • No automated tests needed for static display text
  • Changes are straightforward UI text improvements
  • The Evidence.dev build process will validate markdown syntax

Summary

This is a high-quality, low-risk improvement that enhances the user interface clarity. The changes are:

  • Necessary: Fixes an obvious typo
  • Consistent: Makes metric titles more descriptive and uniform
  • Safe: No functional changes, only display improvements

Recommendation: ✅ Approve

This PR successfully addresses issue #59 and improves the metrics display clarity on the Introduction page.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes metrics display issues on the Introduction page by updating the titles of two key metrics components to be more descriptive and clear.

  • Updated "Pull Requests" title to "Total Pull Requests" for better clarity
  • Updated "Issues" title to "Total Issues" for consistency and clarity

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@edmundmiller

Copy link
Copy Markdown
Contributor Author

@claude now we need to update the SQL statements that are creating the plots.

@claude

claude Bot commented Aug 23, 2025

Copy link
Copy Markdown
Contributor

Claude finished @edmundmiller's task —— View job


Tasks

  • Read pages/index.md to understand current SQL queries and plots
  • Identify SQL statements that need updating to match new metric titles
  • Update SQL queries for consistency with new display names
  • Test changes with npm run sources and npm run build
  • Commit and push changes

Analysis Complete

After thorough analysis of the codebase, I found that no SQL changes are needed. Here's why:

Current Structure is Correct ✅

  1. SQL Queries: The queries in queries/code/growth_gh_prs.sql and queries/code/growth_gh_issues.sql correctly return:

    • num_prs column for pull request data
    • num_issues column for issue data
  2. BigValue Components: The components in pages/index.md correctly reference:

    • value=num_prs for the "Total Pull Requests" metric
    • value=num_issues for the "Total Issues" metric
  3. Display Titles: The "Total" prefix is only in the display titles, not the data column names, which is the correct approach.

Other Chart Titles Are Appropriate ✅

  • pages/code/pull_requests.md: Uses "GitHub Pull Requests over time" (correct for trend charts)
  • pages/code/issues.md: Uses "GitHub Issues over time" (correct for trend charts)
  • These refer to time-series data, not cumulative totals, so "Total" prefix would be inappropriate

Conclusion

The previous changes to the BigValue titles were sufficient. The SQL queries already provide the correct data structure, and the Evidence.dev framework correctly maps the data to the display components. No additional SQL modifications are required.

The metrics display is now properly clarified with "Total Pull Requests" and "Total Issues" while maintaining correct data flow from SQL to visualization.


@mashehu mashehu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uff, this comments are wordy compared to what actually changed 😁

@mashehu mashehu merged commit 691af4e into main Sep 25, 2025
6 checks passed
@mashehu mashehu deleted the claude/issue-59-20250823-0314 branch September 25, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Fix metrics display on Introduction page

3 participants